Range and serialization parity fixes - #44
Merged
Merged
Conversation
Validation and normalization already trim scheme-aware versions, but compareSemver parsed its operands raw, so a padded version fell through to the generic comparator and lost SemVer ordering. Trim in compareSemver and compareCargo, drop the now-redundant compareNPM wrapper, and route npm through compareSemver directly.
Exclude now returns the receiver when the version is outside the range and carries RawConstraints forward when it adds an exclusion. Union keeps an exclusion when the other operand does not contain that version, rather than only when both operands list it. Both Union and Intersect now inherit whichever operand has a non-empty scheme, so combining an untyped range with a typed one yields a typed result regardless of order.
A typed range now serializes under its own scheme regardless of the argument, and scheme aliases (rubygems, golang, elixir, alpine, debian) are mapped to their canonical form in the output.
There was a problem hiding this comment.
Pull request overview
This PR ports several correctness and parity fixes from the Ruby implementation, primarily around range algebra edge-cases, scheme handling, serialization, and semver-ish comparisons.
Changes:
- Adjusts scheme-aware comparison selection and trims whitespace in semver/cargo comparisons (including treating
npmas semver for comparisons). - Fixes range algebra behavior for exclusions, scheme inheritance in set operations, and
Excludesemantics/serialization preservation. - Updates
ToVersStringto canonicalize scheme aliases and to prefer a typed range’s own scheme over the caller-provided scheme; adds regression tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| version.go | Routes npm comparisons through semver comparison logic; keeps scheme canonicalization helper. |
| schemes.go | Trims whitespace in semver and cargo comparisons; removes now-redundant compareNPM. |
| schemes_test.go | Adds regression test ensuring semver-ish comparisons trim whitespace and semver ranges contain trimmed inputs. |
| range.go | Updates Union/Intersect to share schemes, improves exclusion retention rules, and preserves RawConstraints in Exclude. |
| range_test.go | Adds regression tests for Exclude raw-constraint preservation, irrelevant exclusions, exclusion retention in Union, and scheme inheritance. |
| parser.go | Makes ToVersString prefer the range’s scheme and canonicalizes scheme aliases before serialization. |
| parser_test.go | Adds tests for scheme canonicalization and typed-range scheme precedence in ToVersString. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
273
to
277
| // The operands are assumed to use compatible schemes; use UnionChecked to | ||
| // have that verified. | ||
| func (r *Range) Union(other *Range) *Range { | ||
| if r.IsEmpty() { | ||
| return other |
Comment on lines
+294
to
+307
| // An exclusion survives the union only when the other operand does not | ||
| // independently supply the excluded version. | ||
| var exclusions []string | ||
| for _, e := range left.Exclusions { | ||
| if !right.Contains(e) { | ||
| exclusions = append(exclusions, e) | ||
| } | ||
| } | ||
| for _, e := range right.Exclusions { | ||
| if left.Contains(e) || slices.Contains(exclusions, e) { | ||
| continue | ||
| } | ||
| exclusions = append(exclusions, e) | ||
| } |
…arator Union checked IsEmpty before applying the common scheme, so a typed empty operand lost its scheme to the other side. Move the scheme alignment first and return the aligned copy. Union and Intersect now dedup combined exclusions with the scheme comparator instead of string equality, so 1.5 and 1.5.0 collapse to one entry under semver.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ports four correctness fixes from the Ruby implementation:
Range.Excludereturns the receiver unchanged when the version is outside the range, and preservesRawConstraintswhen adding an exclusion.Range.Unionkeeps an exclusion when the other operand does not contain that version. Previously an exclusion survived only if both operands listed it, so(>=1.0 !=1.5) ∪ (>=2.0)wrongly admitted1.5.UnionandIntersectinherit whichever operand has a non-empty scheme, so combining an untyped range with a typed one yields a typed result regardless of order. The unchecked signatures are unchanged; internal callers all combine same-scheme ranges, andUnionChecked/IntersectCheckedremain the API for rejecting mismatched schemes.ToVersStringcanonicalizes the scheme (aliases likerubygems,golang,elixirmap togem,go,hex) and a typed range serializes under its own scheme regardless of the argument.compareSemverandcompareCargotrim their operands so a padded version compares the same as its normalized form;compareNPMis folded intocompareSemver.Each fix has a public-API test that failed before the change.